HDDS-15657. excludeContainersDueToFailure resets every iteration, repeatedly failing containers keep being retried - #10892
Conversation
…eatedly failing containers keep being retried
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for the fix! @sravani-revuri this makes sense for the ContainerNotFoundException cases, which are the permanently broken containers from the Jira but the other two call sites look transient. Could we confirm that every failure stored here is permanent?
sreejasahithi
left a comment
sreejasahithi
left a comment
There was a problem hiding this comment.
Thanks @sravani-revuri for working on this.
Agree that we should only persist ContainerNotFoundException across iterations and not the temp or transient failures.
ashishkumar50
left a comment
ashishkumar50
left a comment
There was a problem hiding this comment.
@sravani-revuri Thanks for the update, please find few nits.
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for reworking! I left two inline comments about one missing production path and its test coverage. Otherwise, the changes look good to me. 🙂
|
|
||
|
|
||
| @Test | ||
| public void testExcludeContainersNotFoundPersistsAcrossIterations() throws Exception { |
There was a problem hiding this comment.
This test may pass before the fix because a missing container already cannot reach moveManager.move() again. Could we also verify that iteration 2 does not call getContainer(notFoundId) again? This would directly test the persistent exclusion behavior.
| // exclude the container which caused failure of move to avoid error in next run. | ||
| selectionCriteria.addToExcludeDueToFailContainers(moveSelection.getContainerID()); | ||
| // exclude the permanently missing container across balancer iterations. | ||
| selectionCriteria.addToExcludeNotFoundContainers(moveSelection.getContainerID()); |
There was a problem hiding this comment.
nit: I noticed thatincSizeSelectedForMoving() (around line 1200) is the one remaining ContainerNotFoundException site that does not record the container. So a container that first goes missing there is probed once more before entering the persistent set.
Should we add it to excludeContainersNotFound there so the next iteration does not probe it again?
4 participants
What changes were proposed in this pull request?
selectionCriteria is re-created fresh every iteration.
If container C failed (ContainerNotFoundException or any other error) in iteration 1 and was added to excludeContainersDueToFailure, it will be tried again in iteration 2 with the same result. This wastes time on permanently broken containers.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15657
How was this patch tested?
Written unit test.